process: improve process.hrtime #10764
Conversation
nodejs-github-bot
added
c++
jasnell
added
the
semver-major
|
Unfortunately because of the change in error condition this has to be labeled as semver-major. |
| const nsec = hrValues[2] - ar[1]; | ||
| return [nsec < 0 ? sec - 1 : sec, nsec < 0 ? nsec + 1e9 : nsec]; | ||
| if (time !== undefined) { | ||
| if (Array.isArray(time) && time.length == 2) { |
I don’t think it has to be. As @joyeecheung mentioned in the PR description, this only adds a throw for situations that we explicitly declared to be undefined behavior in the docs. It’s probably best to consider it breaking, unless there’s a good reason not to do that. |
| const n = conf.n >>> 0; | ||
| const n = conf.n | 0; | ||
| const hrtime = process.hrtime; | ||
| var noDead = hrtime(), i; |
There was a problem hiding this comment.
Can you please make these separate var statements?
|
@jasnell If you still think it is semver-major, I am OK with landing this as semver-major. |
|
I'm more comfortable landing it as semver-major |
|
CI: https://ci.nodejs.org/job/node-test-commit/7257/ (Started this without seeing that one had already been kicked off. Sorry!) |
|
Going to land this if there is nothing else to be addressed. @mscdex Does this LGTY? |
| process.hrtime([]); | ||
| }, /^TypeError: process.hrtime\(\) only accepts an Array tuple$/); | ||
| assert.throws(() => { | ||
| process.hrtime([1]); |
There was a problem hiding this comment.
How about adding another test that has > 2 elements?
|
One nit, but I agree with @jasnell that I would lean more towards semver-major to be on the safe side. Otherwise LGTM. |
* Add benchmarks for diffing a previous result * Improvements to the documentation, including type annotation * Update the outdated comments in src/node.cc, improve comments in lib/internal/process.js * Check the argument is an Array Tuple with length 2
joyeecheung
force-pushed
the
improve-hrtime
branch
from
adda2bc to
0a68dbe
Compare
|
@mscdex Updated, PTAL. |
* Add benchmarks for diffing a previous result * Improvements to the documentation, including type annotation * Update the outdated comments in src/node.cc, improve comments in lib/internal/process.js * Check the argument is an Array Tuple with length 2 PR-URL: #10764 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Brian White <[email protected]>
|
Landed in a647d82 |
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
process
This PR is split into two commits in caution of breaking change, can be squashed when identified not breaking. Non breaking:
Might be breaking: check the length of argument passed into
process.hrtime(). Passing a user-defined array is previously described as undefined behavior in the documentation.Benchmark results: